-
Notifications
You must be signed in to change notification settings - Fork 4
Button2 refactor #177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Button2 refactor #177
Conversation
04dd9ff to
38bee6b
Compare
38bee6b to
52edf80
Compare
deeaf25 to
c8760a8
Compare
d249161 to
1e9244e
Compare
| -- | Using `onPress` on a button multiple times chains the effects together | ||
| -- | from the first applied, out. | ||
| onPress :: forall c. Aff Unit -> ButtonModifier c | ||
| onPress a = propsModifier \props -> props { onPress = props.onPress *> a } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not be necessary but it seemed like it could be useful to build a button with some base behavior and pass it to something else to add its own behavior, a little bit like DOM event bubbling. This seems like the right compose direction to me but I'm curious what anyone else things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this! Maybe if you want we could also expose a more flexible variant like this?
onPress' :: forall c. (Aff Unit -> Aff Unit) -> ButtonModifier c
onPress' f = propsModifier \props -> props { onPress = f props.onPress }Not sure if it would be useful though.
| $ css | ||
| { zIndex: px ziButtonGroup | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this pattern, thanks @arthurxavierx! These styles feel a lot safer bound to a specific component type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caveat here is that modifiers of type forall c. ButtonGroupModifier c can be applied to any component, so maybe this alternative is safer?
type ButtonGroupModifier c = forall r. PropsModifier (buttonGroupComponent :: c | r)And for every component we have (textComponent :: c | r), (buttonComponent :: c | r), etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, true. I wonder if PropsModifier should contain the (component :: c | props).. then it'd be as general as it is now by default so long as c is left open, but locked to a specific component as soon as you set c, and we wouldn't have the redundant XYZModifier types.
Related- I was also wondering if there was a better way to type loadingContent, it didn't fit into ButtonModifier because of the other prop constraints. It works as is, the inconsistent type aliases just seemed a little weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The XYZModifier types are also kind of a weird pattern to me. It's just duck typing actually, because we're saying that such and such modifiers apply to any component that has those props (and only to those), and it breaks in the case where a related component has different props (like with loadingContent).
I think that what we want is primarily a way of constraining modifiers to certain components in a hierarchy, it seems we don't have to care about the props like that, so maybe another way of doing it could be something like this?
type ButtonModifier props = forall c. IsButton c => PropsModifier (component :: c | props)
class IsButton c
instance IsButton Button
instance IsButton LinkButtonNot sure how type inference would be affected by that though…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a phantom type like PropsModifier Button r as well? Going to merge this as is for now though, since it doesn't affect usage much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately PropsModifier cannot have a phantom type parameter because it's a type synonym, so that PropsModifier Button r and PropsModifier Text r would be the same type (unless, of course, that parameter were used in the definition of PropsModifier).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right
arthurxavierx
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I really like some of the helper modifiers like onPress, ariaLabel, submit, etc.
| , hueDisabled :: Color | ||
| , white :: Color | ||
| } | ||
| shade { hue, white, black } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems quite specific to the Button and ButtonGroup components. Maybe we should call it buttonShades or move it somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh haha, I moved it here because it didn't seem buttons specific. Like part of the theme is all our current colors/hues, and this sort of expands each one into a wider range of related colors. I also wondered if it'd be better to move this function into the theme so it can be replaced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I see… I can see the use for hueDarker, hueDarkest, and hueDisabled, but maybe we should grab grey1andgrey2from the theme itself? We alredy haveblack1andblack2`.
(this is totally non-blocking btw, just pure nitpicking)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. We should probably expand all the colors instead of just black if we can do it algorithmically, or leave the theme colors as-is and restrict button color to those predefined ones. The whole theme idea could use some work, like making it available to design and having more things use it so it doesn't look silly as values change, dark mode, etc.
Button2 was one of the first components using emotion and LumiComponent and it shows. Updating it to work more like Text2.
onPressusesAffto control the button loading state, useliftEffectto bypass this behavioronPress,submit,reset, + other button modifiers